Skip to content

fix: preserve operand width in DecimalValue checked arithmetic#7380

Merged
robert3005 merged 7 commits into
vortex-data:developfrom
abnobdoss:fix/decimal-checked-add-no-upcast
Jun 18, 2026
Merged

fix: preserve operand width in DecimalValue checked arithmetic#7380
robert3005 merged 7 commits into
vortex-data:developfrom
abnobdoss:fix/decimal-checked-add-no-upcast

Conversation

@abnobdoss

Copy link
Copy Markdown
Contributor

Summary

Closes: #7022

DecimalValue::checked_add/sub/mul/div unconditionally upcast both operands to i256 and returned DecimalValue::I256, producing unnecessarily wide scalars even when both inputs were narrow (e.g. I32 + I32 → I256).

Operate at max(self, other) width instead, matching the pattern in aggregate_fn/fns/sum/decimal.rs. The old checked_binary_op helper was replaced with a local macro so each op dispatches with its own trait.

AI disclosure: Analysis, implementation, and tests were done with Claude Code under my direction and review.

API Changes

No public API signature changes (verified via ./scripts/public-api.sh).

Overflow is now caught at the target width rather than silently widening (e.g. I8(i8::MAX) + I8(1) now returns None instead of I256(128)). This felt like the most faithful reading of the issue, but I'd appreciate a sanity check that returning None on target-width overflow is the desired semantics rather than, say, promoting to the next wider variant. No in-tree caller depends on the old behavior: sum/mod.rs pre-widens the accumulator by +10 precision, decimal/scalar.rs::checked_binary_numeric requires both operands to share the same width, and sum/constant.rs uses I128 as the multiplier.

Testing

  • Updated test_decimal_value_checked_* to assert the correct variant.
  • Added tests for variant preservation, mixed-width promotion, and overflow at the target width (including i8::MIN / -1).
  • All existing tests pass (212 decimal, 14 sum).

…thmetic (vortex-data#7022)

Signed-off-by: Abanoub Doss <abanoub.doss@gmail.com>
@joseph-isaacs

Copy link
Copy Markdown
Contributor

I256 is not the maximum of the inputs. I would accept a PR doing this

@github-actions

Copy link
Copy Markdown
Contributor

This PR has been marked as stale because it has been open for 14 days with no activity. Please comment or remove the stale label if you wish to keep it active, otherwise it will be closed in 7 days

@github-actions github-actions Bot added the stale This PR is stale and will be auto-closed soon label May 18, 2026
@abnobdoss

Copy link
Copy Markdown
Contributor Author

Hi @joseph-isaacs, sorry not sure I understood - are you saying this change is fine? If so, what would the process be to get this approved / merged?

@github-actions github-actions Bot removed the stale This PR is stale and will be auto-closed soon label May 20, 2026
Comment thread vortex-array/src/scalar/typed_view/decimal/dvalue.rs Outdated
Comment thread vortex-array/src/scalar/typed_view/decimal/dvalue.rs Outdated

@joseph-isaacs joseph-isaacs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this sorry, for delay just a few small ones and we are good to go

claude and others added 4 commits May 21, 2026 00:19
…rt widening cast

Address review feedback on vortex-data#7380:
- Rename `checked_binary_op!` to `checked_widening_binary_op!` to make
  the upcast to the wider operand type explicit at call sites.
- Replace `cast()?` with `vortex_expect`, since widening a
  `DecimalValue` to a type at least as wide as itself cannot fail.

Signed-off-by: Abanoub Doss <abnobdoss@proton.me>
Signed-off-by: Claude <noreply@anthropic.com>
Signed-off-by: Abanoub Doss <abnobdoss@proton.me>
Signed-off-by: Claude <noreply@anthropic.com>
Improve error handling in decimal checked operations
@abnobdoss

abnobdoss commented May 21, 2026

Copy link
Copy Markdown
Contributor Author

Thank you @joseph-isaacs! I've updated the PR per the comments.

Signed-off-by: abnobdoss <abanoubdoss@gmail.com>
@abnobdoss

Copy link
Copy Markdown
Contributor Author

Hi @joseph-isaacs, could you please take a look when you get a chance?

@github-actions

Copy link
Copy Markdown
Contributor

This PR has been marked as stale because it has been open for 14 days with no activity. Please comment or remove the stale label if you wish to keep it active, otherwise it will be closed in 7 days

@github-actions github-actions Bot added the stale This PR is stale and will be auto-closed soon label Jun 13, 2026
@abnobdoss abnobdoss requested a review from a team June 18, 2026 03:42
@abnobdoss

Copy link
Copy Markdown
Contributor Author

Hi @joseph-isaacs, could you please take a look when you get a chance?

@robert3005 robert3005 added changelog/fix A bug fix and removed stale This PR is stale and will be auto-closed soon labels Jun 18, 2026
@robert3005 robert3005 enabled auto-merge (squash) June 18, 2026 09:45
@codspeed-hq

codspeed-hq Bot commented Jun 18, 2026

Copy link
Copy Markdown

Merging this PR will improve performance by 24.74%

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚡ 5 improved benchmarks
✅ 1576 untouched benchmarks
⏩ 1 skipped benchmark1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
Simulation take_10k_random 255.8 µs 197.7 µs +29.39%
Simulation take_10k_contiguous 276.3 µs 218.1 µs +26.65%
Simulation patched_take_10k_contiguous_patches 291 µs 232.1 µs +25.38%
Simulation patched_take_10k_random 303 µs 244 µs +24.18%
WallTime cuda/bitpacked_u8/unpack/3bw[100M] 353.5 µs 298.6 µs +18.38%

Tip

Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.


Comparing abnobdoss:fix/decimal-checked-add-no-upcast (726bfa2) with develop (69ce1ed)

Open in CodSpeed

Footnotes

  1. 1 benchmark was skipped, so the baseline result was used instead. If it was deleted from the codebase, click here and archive it to remove it from the performance reports.

@robert3005 robert3005 merged commit ad46b6c into vortex-data:develop Jun 18, 2026
74 of 78 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/fix A bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DecimalValue checked_add should not upcast to i256 unconditionally

4 participants